Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add History Panel #60

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Conversation

mateusabelli
Copy link

@mateusabelli mateusabelli commented May 21, 2023

✨ Description

This PR will add a history panel to display the previous pasted images from the clipboard. It still at an early stage and it doesn't retrieve the editing state from each image yet. Other than that, everything is quite functional.

To achieve this feature I created a shared state between the component responsible for displaying the image and my new component responsible for displaying the history panel. This implementation might not be according to the project specs, so I'm open for change reviews.

I added comments to help understand my thought process while developing this feature. I am a bit rusty with React, so any feedback is appreciated.

Fixes #13

🖼 Demonstration

prdemo

🧪 To be fixed

  • The delete button does not use the predefined Button component
  • (Fixed) After deleting the current image selected, it remains displaying on the screen
  • After reaching 10 images on the history panel, it won't start overwriting older ones
  • (Fixed) After selecting an image from history, new pastes will be pasted but not displayed automatically
  • (New) Deleting images from history will change the current displayed image, even if the deleted image was not the current displayed one.

📋 Notes

After working with the code for some hours, I learned more about how it worked, how it interacts with the clipboard and how the browser gives a nice and clean URL of the blob.

With this information I don't feel like my comment on the issue #13 would be provide the most elegant solution at the moment.

What I said there could work but so far I'm seeing this simpler solution beating my complex thought back there. I feel like that should be left to be built upon something like this, so there would be much less friction during implementation.

@vercel
Copy link

vercel bot commented May 21, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
screenshot ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 25, 2023 1:01am

Copy link
Collaborator

@iamhectorsosa iamhectorsosa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mateusabelli this is such a great and distinctive feature to have ❤️ Thanks for pursuing this PR. I really feel that we could progressively keep working on enhancing this feature.

The delete button does not use the predefined Button component

This is a minor brush we can give at a later time.

After deleting the current image selected, it remains displaying on the screen

I'm thinking something along these lines. We're keeping state for the current image, what if we compare those string on the click handler and if they match, we reset the current image with the placeholder one?

After reaching 10 images on the history panel, it won't start overwriting older ones

Same here, I would abstract the entire logic into a custom hook and just pass the string. The hook internally should determine what to do with the stored strings and the incoming one. If the max number is reached, it should remove the first item and introduce the incoming one.

function addImageToArray(images, newImage) {
 // Maybe extract this outside of the function
  const MAX_LIMIT = 10;

  // Check if the array has reached the maximum limit
  if (images.length === MAX_LIMIT) {
    // Remove the first image source
    images.shift();
  }

  // Add the new image source to the end of the array
  images.push(newImage);

  return images;
}

After selecting an image from history, new pastes will be pasted but not displayed automatically

This one is a tricky one. I think the problem might be with the useEffect. Either try to separate it into two different effects or I would refactor the component to always grab the src from the state. This would also mean that the pasteImage function should only return the string and then the component should set state and update the ref.

if (imageRef.current){
  currentImage.onclick = async () => {
    // Prolly we should rename that function because it no longer makes sense to pass the ref
    const imageSrc = await pasteImage();
    setCurrentImage(imageSrc);
    imageRef.current.src = imageSrc;
  }
}

Hopefully that makes sense, let me know if you need to talk more about this.

Copy link
Collaborator

@iamhectorsosa iamhectorsosa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this PR ready for review? I tested the latest preview built and I still see opportunity for some improvements. Here are a couple of observations:

  • Order of items: I would expect that the last item in the panel is the latest image pasted from my clipboard. Currently, the first item of the array is the last image pasted. In my opinion, it is a bit confusing.
  • Maxed items doesn't do anything: I would expect that if I reached the maximum number of items in the stored history, the panel would start replacing removing the oldest items to leave space for the recent items to continually keep storing items.
  • Pasting an image that is already there: This might be challenging but if I paste an image that has already been pasted, it creates a duplicate in the array. Are the blobs strings different? Not sure but I think this might be a challenge. If the blobs are always unique we might not be able to control this
  • Storing: We might have to address this in a separate story but the items aren't kept in local storage. This would be a nice feature.

Let me know what challenges you are facing. Also, if you want me to jump in and help with development, please let me know and push your latest changes to this PR. @mateusabelli

@mateusabelli
Copy link
Author

Hello @iamhectorsosa yes its still under development, I'm trying to fix each item in the PR description checkbox, but after some fixes I've found difficulties to extract the shared state to a custom hook and also to refactor the pasteImage function to remove redundant code.

I've left some TODO comments to remind myself of these points, but if you would like to jump in the code I'd be glad to learn how you would approach this.

@iamhectorsosa
Copy link
Collaborator

@mateusabelli push your latest changes to this PR and I'll give it a shot from my end 🙏🏼

@mateusabelli
Copy link
Author

Hi @iamhectorsosa thank you, those are my latest stable changes.

I left comments around the code but if you find anything confusing please let me know.

If you would like, I'm available to continue the PR after we overcome those challenges. Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a panel to display last 10 images from clipboard stored locally
2 participants